Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Pundit adapter blindly ignores existing pundit_user definition #2467

Closed
wants to merge 1 commit into from
Closed

Fix: Pundit adapter blindly ignores existing pundit_user definition #2467

wants to merge 1 commit into from

Conversation

theangryangel
Copy link

As per the pundit readme, there are instances where I need to provide additional context about the user. I do this by defining pundit_user, however the pundit rails_admin adapter ignores this and replaces it with it's own pointing at current_user. This breaks policies that rely on the additional context.

This patch addresses this.

In addition to this I have another issue with the pundit adapter, and I'd like to include some fixes for it as well, but before just doing them I'd like to see if there's a good reason why things have been done the way they are.

Typically the method name for checking with pundit for "can I do X" will be suffixed with a "?". Currently the pundit adapter does not do this.

I see 2 problems with this:

  1. In scenario Missing foreign key displays as null in the list view #1, it means aliasing a bunch of methods (i.e. alias_method :index, :index?) - a minor incovenience
  2. In scenario Values that are null in the list view show a URL #2, I may want different permissions for rails_admin than my standard application policies, for some reason. Currently I'm able to do this because it calls different methods (those without a question mark), but it's not particularly clear that these are specifically for rails_admin. This has confused a junior developer here already.

I'd like to put a patch together that basically either prefixes the methods with rails_admin, or works more like sudosu/rails_admin_pundit where it runs through a specific method, which in turn can use a case statement.

I'd appreciate any input on that before I write a patch and start depending on it.

… definition. This allows additional context as per the pundit readme.
@mshibuya
Copy link
Member

For the scenario 1, that's my fault. I'll change all pundit calls to be suffixed with ?.

For the scenario 2, the recommended way is to set custom authorization key through action configuration.
#2399 (comment)

@theangryangel
Copy link
Author

Thanks for the pointer, @mshibuya. Much appreciated <3

jibidus pushed a commit to sogilis/rails_admin that referenced this pull request Nov 26, 2015
jibidus pushed a commit to sogilis/rails_admin that referenced this pull request Nov 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants